Skip to content

[roottest] Migrate Make to CMake #18596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

guitargeek
Copy link
Contributor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From root-project/roottest#726 (comment):

Please remove this only as the very last step. It is still used(/useable) to clean all the by-product of the 'make' runs that are executed by ctest.

Comment on lines +5 to +8
ROOT_LINKER_LIBRARY(TreeFormulaRetobjGeneration
${CMAKE_CURRENT_SOURCE_DIR}/TreeFormulaRetobjGeneration.cxx
G__TreeFormulaRetobjGeneration.cxx
LIBRARIES Core Tree Hist MathCore)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From root-project/roottest#773 (comment):

Please add the generation of the dictionary as required fixture.

gSystem->Load("libEvent");
TFile *Event = TFile::Open("Event.new.split0.root");
gSystem->Load("libTreeFormulaRetobjGeneration");
TFile *Event = TFile::Open("Event.root");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to avoid this generic file name.

See the discussion in the original PR:
root-project/roottest#773 (comment)

Comment on lines 10 to 16
if(MSVC)
add_custom_command(TARGET IoCompressionGeneration POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>/libIoCompressionGeneration.dll
${CMAKE_CURRENT_BINARY_DIR}/libIoCompressionGeneration.dll
COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>/libIoCompressionGeneration.lib
${CMAKE_CURRENT_BINARY_DIR}/libIoCompressionGeneration.lib)
endif()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From root-project/roottest#774 (comment):

@bellenot Is that an okay solution?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From root-project/roottest#774 (comment):

@bellenot Is that an okay solution?

Almost. It should be:

if(MSVC AND NOT CMAKE_GENERATOR MATCHES Ninja)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, problem can be resolved using ROOTTEST_GENERATE_DICTIONARY macro.

Comment on lines 5 to 8
ROOT_LINKER_LIBRARY(IoCompressionGeneration
${CMAKE_CURRENT_SOURCE_DIR}/IoCompressionGeneration.cxx
G__IoCompressionGeneration.cxx
LIBRARIES Core Tree Hist MathCore Physics Graf Matrix)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From root-project/roottest#774 (comment):

I recommend adding the ROOT_GENERATE_DICTIONARY as a FIXTURES_REQUIRED

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is ROOTTEST_GENERATE_DICTIONARY macro which supports FIXTURES_REQUIRED.
Example can be found in roottest/root/io/json sub-dir

Comment on lines 18 to 21
ROOTTEST_GENERATE_EXECUTABLE(IoCompressionGenerator
${CMAKE_CURRENT_SOURCE_DIR}/IoCompressionGenerator.cxx
LIBRARIES Core RIO Net Tree Hist MathCore IoCompressionGeneration
DEPENDS IoCompressionGeneration
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From root-project/roottest#774 (comment):

This actually needs to be a FIXTURES_REQUIRED rather than a DEPENDS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also consider this previous discussion on root/io/perf:

root-project/roottest#732 (comment)

Copy link

github-actions bot commented May 4, 2025

Test Results

    19 files      19 suites   3d 16h 29m 4s ⏱️
 2 837 tests  2 826 ✅   0 💤  11 ❌
52 405 runs  51 968 ✅ 302 💤 135 ❌

For more details on these failures, see this check.

Results for commit a8cd512.

♻️ This comment has been updated with latest results.

@pcanal
Copy link
Member

pcanal commented May 6, 2025

As another general reminder, one challenge in the migration from the Makefile to a CMakeLists.txt is to ensure that all the implicitly generated Makefile based test (i.e. those without an explicit target) are actually migrated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants